Update MSC4311 tests to conform to the spec and better coverage#796
Update MSC4311 tests to conform to the spec and better coverage#796turt2live wants to merge 21 commits into
Conversation
| must.MatchGJSON(t, ev, | ||
| match.JSONKeyPresent("origin_server_ts"), | ||
| ) | ||
| srv.Mux().HandleFunc("/_matrix/federation/v2/invite/{roomID}/{eventID}", srv.ValidFederationRequest(t, func(fr *fclient.FederationRequest, pathParams map[string]string) util.JSONResponse { |
There was a problem hiding this comment.
You should be able to alternatively use:
srv := federation.NewServer(t, deployment,
federation.HandleInviteRequests(func(p gomatrixserverlib.PDU) {
// checks here
}),
federation.HandleKeyRequests(),
federation.HandleMakeSendJoinRequests(),
federation.HandleTransactionRequests(nil, nil),
)
There was a problem hiding this comment.
I went down this route but the problem is that federation.HandleInviteRequests(...) doesn't allow you to inspect the invite_room_state yet. It would have to be updated to pass in the full invite request to the callback.
And gomatrixserverlib needs to be updated to handle the new world where invite_room_state can be stripped state or full PDUs.
We could have an infallible InviteV2Request.StrippedInviteRoomState() that would give a view of stripped state regardless of whether we were passed stripped or full PDU's (we can derive stripped state from the full PDU's). And then a fallible InviteV2Request.FullInviteRoomState() that would return an error if the homeserver didn't pass us full PDUs.
We would also need to update IRoomVersion to add new fields like CreateEventRequiredInInviteRoomState and FullPDUInviteRoomStateMSC4311InviteRoomState
If any of the events are not a PDU, not for the room ID specified, or fail signature checks, or the
m.room.createevent is missing, the receiving server MAY respond to invites with a400 M_MISSING_PARAMstandard Matrix error (new to the endpoint). For invites to room version 12+ rooms, servers SHOULD rather than MAY respond to such requests with400 M_MISSING_PARAM.
But it doesn't seem possible to see. For example, Synapse strips it out when trying to view events, https://github.com/element-hq/synapse/blob/6100f6e4f7fb0c72f1ae2802683ebc811c0e3a77/synapse/events/utils.py#L590-L596
| // TODO: Test events not full PDU's | ||
|
|
||
| // TODO: Test events not from the same room | ||
|
|
||
| // TODO: Test invalid signatures/hashes |
There was a problem hiding this comment.
Something for future PRs
| // > error (new to the endpoint). For invites to room version 12+ rooms, servers | ||
| // > SHOULD rather than MAY respond to such requests with `400 M_MISSING_PARAM`. | ||
| func TestMSC4311RejectInvalidStrippedStateFederation(t *testing.T) { | ||
| runtime.SkipIf(t, runtime.Synapse) // FIXME: Run these tests after 2027-06-01 |
There was a problem hiding this comment.
Also cross-linked this in the Synapse codebase (see element-hq/synapse#19723)
MadLittleMods
changed the title
MadLittleMods
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates MSC4311 coverage for room version 12 by replacing an incorrect stripped-state test with client-server, federation, and rejection coverage, plus helper support for knock flows.
Changes:
- Adds MSC4311 tests for client
/syncstripped state and federation invite/knock stripped state. - Adds
JSONArraySomematcher for asserting that at least one JSON array element matches. - Adds client and federation knock helpers used by the new tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/v12_test.go |
Replaces the old MSC4311 test with expanded client, federation, and invalid-state rejection tests. |
match/json.go |
Adds JSONArraySome matcher for “any element matches” assertions. |
federation/server.go |
Adds federation knock helper and strict knock_room_state validation option. |
client/client.go |
Adds client knock helper and fixes invite helper comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var satisifed bool = false | ||
| body.ForEach(func(_, val gjson.Result) bool { | ||
| err := fn(val) | ||
| satisifed = err != nil |
| } | ||
|
|
||
| // JSONArraySome returns a matcher which will check that `wantKey` is an array then | ||
| // loops over each item calling `fn`. If `fn` returns nil, the matcher is satisifed, |
| serverNameStrings[i] = string(serverName) | ||
| } | ||
| query := url.Values{ | ||
| "via": serverNameStrings, |
Update MSC4311 tests to conform to the spec. MSC4311 states that the client API should still use the stripped state event format. For the federation API, it should use full PDU's. MSC4311 also requires the
m.room.createevent be included in the list of stripped state."Stripped state" is a generic term that refers to simplified slice of state shared in
invite_room_state/knock_room_state(invite_state/knock_statewith/sync) before someone is joined to a room and can see the full room state.This PR was originally authored by @turt2live taking a stab at fixing single the flawed test (self-reported:
"I have no idea what I'm doing" disclaimer goes here) but has since been taken over by @MadLittleMods. And has naturally evolved some new better test coverage as I've worked on a full solution.Synapse PR:
element-hq/synapse#18822element-hq/synapse#19723The previous tests passed because of a flawed implementation in Synapse which is being removed in element-hq/synapse#19723 as well.
Dev notes
Running the tests with Synapse: